Remove `Function$` entirely. by cristianoc · Pull Request #7200 · rescript-lang/rescript

cristianoc · 2024-12-12T09:58:49Z

Change the AST and compiler back-end so that Function$ is not used anymore to encode function definitions.

cristianoc · 2024-12-13T12:49:28Z

 module V4C = {
  @res.jsxComponentProps
-  type props = {}
+  type props<'a, 'b> = {a: 'a, b: 'b}


@mununki are these changes good or bad?

Add test to analysis (so it's compiled and uses rescript-react as a dependency) that these compile.
Tested that the same example did not compile on master, so this is the intended transformation that was somehow lost along the way sometime.

de23262

cristianoc · 2024-12-15T10:00:39Z

@cknitt we can move to testing phase with this.
Eg fix the issue you found.
Once fixed, I have some implementation cleanup left, but would like to add any missing tests first.

github-actions

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Syntax Benchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.05.

Benchmark suite	Current: `7d4af4a`	Previous: `e1b7fb7`	Ratio
`Parse RedBlackTree.res - time/run`	`1.3137930666666666` ms	`1.2123143266666667` ms	`1.08`
`Parse Napkinscript.res - time/run`	`43.116282813333335` ms	`39.28006235333333` ms	`1.10`
`Parse HeroGraphic.res - time/run`	`6.496717546666666` ms	`5.13472718` ms	`1.27`

This comment was automatically generated by workflow using github-action-benchmark.

cristianoc · 2024-12-16T08:00:39Z

@cknitt all the issues are resolved now, did you test this entirely on one project already?
That's the kind of confirmation needed before moving on with this.

cknitt · 2024-12-16T09:57:30Z

@cknitt all the issues are resolved now, did you test this entirely on one project already?

Yes, did that before, and now just retested with the last commit. Everything builds fine.

However, I found that the editor tooling crashes:

Error: Command failed: /Users/christoph/.vscode/extensions/chenglou92.rescript-vscode-1.58.0/server/analysis_binaries/darwinarm64/rescript-editor-analysis.exe hover /Users/christoph/projects/cca/inform4/app-common/src/config/ConfigLoader.res 6 16 /var/folders/l3/d1wj17dj2z321tjq9s9fnlsh0000gn/T/rescript_format_file_8986_12 true
    at genericNodeError (node:internal/errors:984:15)
    at wrappedFn (node:internal/errors:538:14)
    at checkExecSyncError (node:child_process:925:11)
    at Object.execFileSync (node:child_process:961:15)
    at Object.func (node:electron/js2c/node_init:2:2559)
    at Lt (/Users/christoph/.vscode/extensions/chenglou92.rescript-vscode-1.58.0/server/out/cli.js:36:5456)
    at Ct (/Users/christoph/.vscode/extensions/chenglou92.rescript-vscode-1.58.0/server/out/cli.js:36:5658)
    at aw (/Users/christoph/.vscode/extensions/chenglou92.rescript-vscode-1.58.0/server/out/cli.js:42:5630)
    at process.Ph (/Users/christoph/.vscode/extensions/chenglou92.rescript-vscode-1.58.0/server/out/cli.js:42:15244)
    at process.emit (node:events:518:28) {
  status: null,
  signal: 'SIGSEGV',
  output: [ null, <Buffer >, <Buffer > ],
  pid: 9733,
  stdout: <Buffer >,
  stderr: <Buffer >

cknitt · 2024-12-16T09:59:22Z

Ok, that seems to be using the analysis exe that comes with the VS Code plugin for some reason.

cristianoc · 2024-12-16T15:10:16Z

Ok, that seems to be using the analysis exe that comes with the VS Code plugin for some reason.

Great, this PR is ready to go then.
Picking up the right binary is for the TypeScript part for the editor plugin: @zth

cknitt · 2024-12-16T16:47:55Z

Picking up the right binary is for the TypeScript part for the editor plugin: @zth

This is already (mostly) done, but not merged yet: rescript-lang/rescript-vscode#1055

The arity attribute goes on the `Function$` ast node, not the `Pexp_fun` one.

cristianoc · 2024-12-18T11:05:09Z

This could be squash merged, but it would mess up rebasing #7195
That's a complex PR: won't take risks of introducing issues while merging. I'll rebase merge this.

cristianoc force-pushed the remove_Function$ branch from f1f3b71 to 79a3876 Compare December 12, 2024 09:59

cristianoc changed the title ~~Remove function$~~ wip remove Function$ Dec 12, 2024

cristianoc force-pushed the remove_Function$ branch 4 times, most recently from fca8ee2 to 01e5837 Compare December 12, 2024 16:54

cristianoc commented Dec 13, 2024

View reviewed changes

Comment thread tests/analysis_tests/tests-reanalyze/termination/expected/termination.txt Outdated

Comment thread tests/analysis_tests/tests/src/expected/DocumentSymbol.res.txt

cristianoc changed the title ~~wip remove Function$~~ Remove Function$ entirely. Dec 13, 2024

cristianoc requested review from cknitt and zth December 13, 2024 13:40

cknitt reviewed Dec 13, 2024

View reviewed changes

Comment thread tests/syntax_tests/data/parsing/errors/expressions/expected/array.res.txt

Comment thread tests/syntax_tests/data/parsing/errors/expressions/expected/consecutive.res.txt

cristianoc commented Dec 15, 2024

View reviewed changes

Comment thread tests/tools_tests/ppx/TestPpx.res

github-actions Bot reviewed Dec 15, 2024

View reviewed changes

zth approved these changes Dec 18, 2024

View reviewed changes

cristianoc added 9 commits December 18, 2024 11:59

wip remove Function$

e7e9562

printer

5a3234a

Update printtyped.ml

ecc5dec

Adjustments to lambda translation.

7a18bdf

remove infinte loop in builtin ppx

56a2a18

Fix derive projector arity.

a8250da

Update TestPpx.res.jsout

48eb846

analysis

44bb8a9

TODO: investigate termination

429a147

cristianoc added 8 commits December 18, 2024 11:59

Clean up now unused occurrences of Function$.

a970164

Add ppx error test

e356454

Adapt ppx ast conversion that was still missing.

0d82a3e

Remove what's now the identity function.

ee7484d

Fix ppx mapper and add example.

816e61c

The arity attribute goes on the `Function$` ast node, not the `Pexp_fun` one.

Remove difference on termination analysis tests.

bb03d23

Added test that components with type vars do compile.

b2638f8

Update CHANGELOG.md

0196c82

cristianoc force-pushed the remove_Function$ branch from 0c15ebc to 0196c82 Compare December 18, 2024 11:03

cristianoc merged commit 4f961ab into master Dec 18, 2024

cristianoc deleted the remove_Function$ branch December 18, 2024 11:14

Conversation

cristianoc commented Dec 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

cristianoc Dec 13, 2024

Choose a reason for hiding this comment

Uh oh!

cristianoc Dec 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cristianoc commented Dec 15, 2024

Uh oh!

Uh oh!

github-actions Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

⚠️ Performance Alert ⚠️

Uh oh!

cristianoc commented Dec 16, 2024

Uh oh!

cknitt commented Dec 16, 2024

Uh oh!

cknitt commented Dec 16, 2024

Uh oh!

cristianoc commented Dec 16, 2024

Uh oh!

cknitt commented Dec 16, 2024

Uh oh!

cristianoc commented Dec 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cristianoc commented Dec 12, 2024 •

edited

Loading

cristianoc Dec 16, 2024 •

edited

Loading

github-actions Bot left a comment •

edited

Loading